-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Read More block: add aria-label and screen reader text #45490
Conversation
Open in CodeSandbox Web Editor | VS Code | VS Code Insiders |
Thank you for working on this Jeff. Did you intend to use the Post ID only if there is no post title? |
Ah I see, thank you for explaining, I'm quite new to working on accessibility issues.
I used the post ID here because of the comment in the original issue:
Maybe I misunderstood the feedback, and it would make sense to return a better description (e.g. |
It would solve part of the issue if the default visible text was changed from "Read more" to "Read more about post title". The problem with that is that it would change the text for users who are using the old default and it may be unexpected for them. Just brainstorming:
If we are choosing between aria-label and screen reader text, I think that screen reader text may help more people since it is also visible, even if only when the link is focused? |
👍 I will update the PR accordingly. |
409da8d
to
12c24f7
Compare
The reference "pointing to the title by ID" refers to the mechanism of using
That creates a relationship between 'read more' and the post title so that the link is described by the title, and the reference is to the ID attribute of the post title. Using screen reader text is, however, probably a better solution, as it is supported in a broader capacity and easier to make visible when needed. |
Thanks for the clarification. Would the redundancy of having both |
No, only provide one option. Having both, e.g.
would result in reading "Read more about Post Title [pause] Post Title"; the redundancy would mostly just be annoyingly verbose. |
VoiceOver reads this change as:
(I do not hear a pause, but I am not a screen-reader user) |
Thanks for the feedback and bearing with me! I pushed a commit that attempts to make the screen reader text more compatible with all scenarios, using a colon instead of "Read more about" to avoid the redundancy — what do you think? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's looking good! Some changes still need consideration.
$justify_class_name = empty( $attributes['justifyContent'] ) ? '' : "is-justified-{$attributes['justifyContent']}"; | ||
$wrapper_attributes = get_block_wrapper_attributes( array( 'class' => $justify_class_name ) ); | ||
$more_text = ! empty( $attributes['content'] ) ? wp_kses_post( $attributes['content'] ) : __( 'Read more' ); | ||
return sprintf( | ||
'<a %1s href="%2s" target="%3s">%4s</a>', | ||
'<a %1s href="%2s" target="%3s">%4s<span class="screen-reader-text">%5s</span></a>', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There needs to be a space between the visible link text and the hidden text, otherwise the words will be pasted together. e.g. <a %1s href="%2s" target="%3s">%4s<span class="screen-reader-text"> %5s</span></a>
See the space inside the screen-reader-text
span.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make "grammatical" sense in cases where the user has customized the "read more" text, I went with the following format:
Read More: Post Title
Does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. Also good for internationalization.
/* translators: %s is either the post title or post ID to describe the link for screen readers. */ | ||
$screen_reader_text = sprintf( | ||
__( ': %s' ), | ||
$post_title !== '' ? $post_title : __( 'post ' ) . $post_ID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the use case where the post has no title? The post ID does differentiate links, but not in any way that's useful to the user, so I'm wondering if there's a better fallback option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I'm not sure. We could use aria-describedby
in this case, but again not sure if this is any more useful because it's linking the read more to a post that has no title.
Would it be better to state that there is no post title, e.g. Read More: (post with no title)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the scenario where an implementation uses primarily posts with no titles (ugly, but I've seen it), that would result in a series of identical links. Perhaps a combination, e.g. Read More: untitled post $post_ID
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a combination, e.g. Read More: untitled post $post_ID
Done in 369691d
369691d
to
d98f6ad
Compare
@alexstine would you be able to give this a review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good and it's working well in testing on VoiceOver/Safari. When post has a title, it's reading "Read more: post title". If there's no title, it reads "Read more: untitled post 234".
@tellthemachines Thanks for testing/reviewing, I have been swamped with day job stuff lately. @jffng Awesome improvement, thanks. |
$screen_reader_text = sprintf( | ||
/* translators: %s is either the post title or post ID to describe the link for screen readers. */ | ||
__( ': %s' ), | ||
'' !== $post_title ? $post_title : __( 'untitled post ' ) . $post_ID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We received this feedback related to this change https://core.trac.wordpress.org/ticket/57471#comment:27
Basically we should avoid concatenation when it comes to translated strings.
Let's make sure to follow-up on that and label the PR with the "backport to WP beta/RC" label to include on the next package update in core.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tracked here #47812
What?
This PR adds
anaria-label
and.screen-reader-text
to the output of the Read More block. I could use some input as to whether the resulting markup is acceptable.Why?
Fixes 1/2 of #45396
https://make.wordpress.org/themes/handbook/review/accessibility/required/#repetitive-link-text
How?
See above.
Testing Instructions
Screenshots or screencast